Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fixed bug not sending all pending messages #44

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fixed bug not sending all pending messages #44

wants to merge 1 commit into from

Conversation

dirkmb
Copy link

@dirkmb dirkmb commented Sep 20, 2023

In my application I always flag (setting sendRequest to true) 8 PDOs for sending at the same time and I was wondering why only the first 3 - 4 PDOs are transmitted.

If the CO_CANinterrupt_TX is not able to send out all pending messages (e.g. if there are more pending messages than there are free CAN tx mailboxes) the CANtxCount is set to 0, which prohibits the sending of theses pending messages in the next interrupt.

I also stop the iteration if sending of one pending message fails, because following messages can also not be send if the buffers are already full.

I could not find an issue for this bug, if you like I can open one.

@Florioo
Copy link

Florioo commented Mar 14, 2024

This has solved most of my issues in a project im working on (using STM32G473).

I was having similar issues where the sdo client / sdo_server got locked up because the TX buffer was full.
More specifically the bufferFull was was not cleared because the packet was never sent, this causes the entire module to lock up.

Since implementing this patch the canopen stack is quite stable.

@CANopenNode
Copy link
Owner

I'm not sure about this solution. Stack must be fully stable, no messages lost, nothing. Here is similar code for PIC32, which makes no problems:
https://github.com/CANopenNode/CANopenPIC/blob/master/PIC32/CO_driver.c#L771

ESFDH
ESFDH approved these changes Jun 7, 2024
View reviewed changes
Copy link

@ESFDH ESFDH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I encountered a similar issue in my project, where only the first 4 messages are sent when I need to send 6 TPDOs simultaneously. and I made the same modifications.
The STM32 has 3 CAN transmit mailboxes. When 6 messages are sent simultaneously, the first 3 messages are successfully placed into the mailboxes. After the first message is sent successfully, HAL_CAN_TxMailbox0CompleteCallback is called, then CO_CANinterrupt_TX is entered. At this point, mailbox 1 is empty, and mailboxes 2 and 3 are not empty. Message 4 is then placed into mailbox 1. Messages 5 and 6 are not sent because there are no available mailboxes. If the code does not break at this point, CANmodule->CANtxCount will be set to 0, and messages 5 and 6 will not be sent even when mailboxes become available.

@HamedJafarzadeh HamedJafarzadeh self-assigned this Jun 7, 2024
@dirkmb
Copy link
Author

dirkmb commented Dec 10, 2024

I'm not sure about this solution. Stack must be fully stable, no messages lost, nothing. Here is similar code for PIC32, which makes no problems: https://github.com/CANopenNode/CANopenPIC/blob/master/PIC32/CO_driver.c#L771

the code you cited also includes the missing break statement... so the PIC code is correct, the STM code is not. Actually the PIC code always just sends one message at a time (the break is always executed if a message has been send).

I removed the CANModule_local->CANtxCount = 0U; because the counter has already been decremented if a message has successfully been send. So it is already 0 if the previous lines have successfully send all pending messages. If the messages could not send it's wrong to clear the counter, because that's the actual bug that looses the messages.

@dirkmb dirkmb mentioned this pull request Dec 10, 2024
Open
@HamedJafarzadeh
Copy link
Collaborator

Good catch ! Looking at the code, I think you are right. I haven't test it myself but I believe it is a correct change, From my side, we can merge it in. @MaJerle do you approve as well ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ESFDH ESFDH ESFDH approved these changes

@Florioo Florioo Florioo approved these changes

Assignees

@HamedJafarzadeh HamedJafarzadeh

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dirkmb @Florioo @CANopenNode @HamedJafarzadeh @ESFDH